fix(sdk): generate titles with Responses and subscription streaming - #4968

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Review

🟢 Good taste — clean, minimal, well-tested.

The PR makes two focused changes:

1. Title generation API dispatch (title_utils.py): Routes Responses-capable models to llm.responses(messages, store=False) and others to llm.completion(messages). The streaming guard if llm.stream and not llm.requires_streaming correctly preserves mandatory streaming for subscription models (which reject non-streaming requests) while disabling it for regular streaming models that have no on_token callback wired during title generation. The store=False parameter is appropriate — title generation should not persist server-side.

2. API breakage checker (check_sdk_api_breakage.py): Converts _ACCEPTED_REMOVED_MEMBERS from a frozenset to a dict with per-member reason strings, and adds LLM.modify_params as an accepted removal (removed in PR #4954 after its deprecation runway). This is a clean improvement — the diagnostic print now uses the per-member reason instead of a hardcoded message for all entries.

Verification

• All 77 tests pass (12 title-generation + 65 breakage-checker).
• The new test_title_uses_real_http_transport test is excellent: it spins up a loopback HTTP server and exercises all three modes (chat, responses, subscription) without mocking the LLM transport, verifying both API endpoint dispatch and streaming behavior.
• The breakage test correctly verifies exact membership, negative cases (wrong feature, wrong package), and output content.

Risk Assessment

🟢 LOW — Title generation is a background cosmetic operation that does not affect agent reasoning, tool use, planning, memory, or terminal handling. The same prompt goes to the same model; only the API endpoint path changes for Responses-capable models. Not in the eval-risk category.

Verdict: ✅ Worth merging

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

This PR makes two changes:

1. Title generation dispatch (title_utils.py): Routes Responses-capable models to llm.responses(messages, store=False) instead of llm.completion(), and preserves streaming for subscription models that requires_streaming.
2. CI tooling (check_sdk_api_breakage.py): Adds LLM.modify_params to the accepted-removals list and converts the data structure from frozenset to dict to carry per-member acceptance reasons.

Analysis

Title generation (title_utils.py)

The change correctly handles three cases:

• Chat completions models: uses_responses_api() returns False, so completion() is used. Streaming is disabled via model_copy when not requires_streaming (same as before).
• Responses-capable non-subscription models: uses_responses_api() returns True, so responses(store=False) is called. Streaming is disabled first (since requires_streaming is False), and responses() also has an internal fallback that disables streaming when on_token is None — double-safe.
• Subscription models: requires_streaming is True, so streaming is NOT disabled. responses(store=False) is called with stream=True from self.stream, and the stream is drained internally by the _one_attempt logic without needing an on_token callback. This is correct — subscription endpoints reject non-streaming requests.

Both completion() and responses() return LLMResponse with a normalized Message, so the title extraction code (response.message.content[0]) works identically for both paths. Verified via _build_responses_result which calls Message.from_llm_responses_output().

The store=False parameter is appropriate for a stateless title-generation call. For subscription models, create_llm already sets litellm_extra_body={"store": False}, and select_responses_options sets out["store"] = False from the explicit parameter — no conflict since both are False.

CI tooling (check_sdk_api_breakage.py)

Clean refactor: frozenset to dict to allow per-member acceptance reason strings. The _is_accepted_removed_member lookup ((package, feature) in _ACCEPTED_REMOVED_MEMBERS) works identically for both types. The diagnostic message now pulls the reason from the dict value instead of hardcoding it. The new test verifies exact matching (rejects wrong package, wrong member name) and checks the diagnostic output contains "PR #4954".

Tests

The new test_title_uses_real_http_transport test is well-structured — it uses a real loopback HTTP server (no mocked LLM transport) and parametrizes across all three modes (chat, responses, subscription). Assertions verify the correct endpoint path, streaming behavior, and store=False for responses/subscription modes.

Risk Assessment

🟢 LOW — This is a focused bugfix to background title-generation utility code (not the agent reasoning loop, tool calling, planning, or condenser behavior). The CI tooling change is a data-structure refactor with exact-match tests. No breaking changes to public APIs, no event type modifications, no security concerns.

The title-generation path is a side-channel call that runs after the first user message; it does not affect the agent's core reasoning loop or benchmark/eval performance. However, since the repo review guidelines are strict about anything adjacent to agent behavior, I'm leaving a COMMENT rather than APPROVE — a human maintainer should make the final call on whether lightweight eval confirmation is needed.

Verdict: Code is correct and clean. No material issues found.

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

🟢 Good taste — minimal, focused change that correctly dispatches title generation through the right API path for each model type.

Summary

The PR makes two changes to generate_title_with_llm:

1. Streaming guard now exempts requires_streaming models — subscription models reject non-streaming requests, so the previous unconditional stream=False would break them. The responses() method already has the same requires_streaming exemption internally (llm.py:1763), so this is consistent.

2. Responses API dispatch — Responses-capable models now use llm.responses(messages, store=False) instead of llm.completion(messages), with store=False to avoid persisting transient title-generation calls server-side.

Cross-file verification

• LLM.responses() (llm.py:1762) has the same streaming guard as completion() but with the requires_streaming exemption, so subscription models correctly stay in streaming mode with internal drain. ✓
• LLM.completion() (llm.py:1533) does not have the requires_streaming exemption — it unconditionally degrades to non-streaming when no on_token is provided. This is pre-existing and not introduced by this PR. In practice, subscription models use the Responses API, so this path isn't hit for them.
• uses_responses_api() (llm.py:2530) respects explicit api_mode and falls back to model-feature capability detection for auto mode. ✓

Testing

The new test_title_uses_real_http_transport test is well-structured — it uses a real loopback HTTP server instead of mocked LLM transport, covering all three modes (chat, responses, subscription) and asserting on the actual request path, streaming flag, and store=False. This is a real code-path test that would catch regressions.

Risk Assessment

🟢 LOW — Title generation is a cosmetic side-channel that doesn't affect the agent reasoning loop, tool execution, or any benchmark-relevant behavior. No eval evidence needed.

Verdict: ✅ Worth merging — clean, correct, well-tested.

LGTM

I’m not sure why streaming was disabled. I guess the question is, does it work now? For ChatGPT subscription.

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

Clean refactoring that moves Responses/Chat-Completions dispatch from make_llm_completion/amake_llm_completion utility functions into LLM.generate()/LLM.agenerate() methods on the LLM class itself. All callers migrated to direct generate()/agenerate() calls. Agent-only policy (add_security_risk_prediction=True, store=False) is now explicit at the agent call site rather than baked into a shared wrapper. Title generation no longer force-disables streaming, which correctly enables mandatory streaming for subscription models.

Analysis

Design: Good taste — the dispatch logic belongs on LLM (which owns uses_responses_api()), not in a utility function in agent/utils.py. The generate/agenerate methods are thin dispatch wrappers with **kwargs forwarding, matching the existing completion/responses signatures. The separation of agent-only add_security_risk_prediction from generic generation is the right call.

Behavioral changes verified as safe:

• tools=None (new) vs tools=[] (old make_llm_completion default): both are falsy, so completion's if tools: and responses's if tools else None produce identical behavior.
• Title generation streaming: old code did model_copy(update={"stream": False}) + completion(). New code calls generate() which lets completion/responses handle the streaming fallback gracefully (on_token=None -> non-streaming), except for requires_streaming models (subscription) where streaming is mandatory. The real HTTP transport test (test_title_uses_real_http_transport) covers all three modes.
• ask_agent no longer passes add_security_risk_prediction=True: correct — it is a sidebar question, not an agent tool-calling path.

Removed functions: make_llm_completion/amake_llm_completion were never exported from __init__.py and no remaining references exist. No public API break.

Tests: The new dispatch tests (test_generate_dispatches_to_configured_api, test_agenerate_dispatches_to_configured_api), the real HTTP transport title test, and the agent step kwargs verification are meaningful and exercise real code paths. The live server test mock updates correctly separate title calls from agent calls.

Risk Assessment

MEDIUM — This PR changes the LLM dispatch path for agent steps, condensation, hook evaluation, cleanup, vision inspection, Ask Oracle, and title generation. While the refactoring is behavior-preserving by design and the changes are well-tested, any change to the agent LLM call path could plausibly affect benchmark/evaluation performance. Per the repo eval-risk policy, I am not approving and flagging this for a human maintainer to decide after running lightweight evals.

Recommendation: Run a lightweight eval (e.g., a small SWE-bench or GAIA subset) to confirm no regression before merging.

This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.

Summary

The PR introduces LLM.generate()/LLM.agenerate() as a generic dispatch method, removes make_llm_completion/amake_llm_completion, and migrates all callers to direct generate()/agenerate() calls. The design is clean — API-mode selection lives in the LLM abstraction, agent-specific policy (add_security_risk_prediction=True, store=False, tools, callbacks) stays explicit at call sites. The title-generation transport test using a real HTTP server is excellent.

However, there is one material correctness bug in the telemetry changes, and the PR touches agent step logic, condenser behavior, and hook evaluation without eval evidence.

🔴 Critical Issue

telemetry.py: __dict__.get() breaks cache token accounting for Pydantic extra fields

The three getattr(usage, ..., 0) calls were replaced with usage.__dict__.get(..., 0) / p_details.__dict__.get(..., 0) to remove them from the forbidden dynamic attributes baseline. But litellm's Usage model uses extra="allow", so dynamically-set fields like cache_read_input_tokens and cache_creation_input_tokens are stored in __pydantic_extra__, not in __dict__. This means __dict__.get("cache_read_input_tokens", 0) always returns 0.

Verified empirically:

u = Usage.model_validate({
    'prompt_tokens': 1000, 'cache_read_input_tokens': 800, 'cache_creation_input_tokens': 200
})
getattr(u, 'cache_read_input_tokens', 0)  # → 800 (correct)
u.__dict__.get('cache_read_input_tokens', 0)  # → 0 (broken)

This silently zeroes out Anthropic prompt-caching token counts in telemetry/metrics for both cache reads and cache writes. The same issue affects p_details.__dict__.get("cache_creation_tokens", 0) when cache_creation_tokens is an extra field on the details model.

The _cache_creation_input_tokens change (using isinstance(usage, Usage) + direct attribute access) is fine — that field is a declared PrivateAttr on Usage, so direct access works and won't raise AttributeError.

Suggested fix: Use __pydantic_extra__ or keep getattr and accept the baseline entry. Alternatively, model_dump() would include extra fields but is heavier.

Eval Risk

This PR changes agent step dispatch (agent.py), condenser LLM calls (llm_summarizing_condenser.py), hook evaluation (executor.py), and cleanup profile behavior — all of which could plausibly affect benchmark/evaluation performance. No eval monitor link or human eval confirmation is provided in the PR description. Flagging for a human maintainer to decide after running lightweight evals.

Risk Assessment

• Overall PR: 🟡 MEDIUM
• The telemetry bug is a silent regression in cost/token accounting that won't crash but produces incorrect metrics.
• The agent behavior changes are functionally equivalent to the previous make_llm_completion wrapper (same dispatch logic, same parameters), so the risk of behavioral regression is low — but eval validation is warranted given the scope.
• The CI baseline-shrink guard and the stream_context.py try/except change are clean and correct.

Verdict

COMMENT — the telemetry __dict__.get bug should be fixed before merge, and eval validation is recommended given the scope of agent/condenser/hook changes.